fix(cli): v1 config rejecting endpoints without explicit port#1576
Conversation
State the port check's single invariant directly: `--port 0` (let the OS pick) is incompatible with a `--pj-endpoint` URL that pins a specific port.
Coverage Report for CI Build 26398782191Coverage increased (+0.05%) to 85.192%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
caarloshenriq
left a comment
caarloshenriq
left a comment
There was a problem hiding this comment.
tACK
The condition rewrite from pj_endpoint.port().is_none() != (v1.port == 0) to v1.port == 0 && v1.pj_endpoint.port().is_some() reads much more clearly and the tests confirm each case: random port rejected only when the endpoint has an explicit port, and allowed when it does not
| fn accepts_explicit_port_with_different_explicit_endpoint_port() { | ||
| Config::new(&cli("3000", "https://example.com:443/")).unwrap(); | ||
| } |
There was a problem hiding this comment.
What is the expected behavior here? Should the config override the existing endpoint port?
There was a problem hiding this comment.
Expected behavior - the test should not fail, because this is a valid configuratin combination.
On the precedence between the config file and CLI args - I checked locally that CLI args have higher priority. If port and/or pj_endpoint are set in both the config file and CLI args, the CLI values are used.
I don't think we should rewrite the port inside pj_endpoint from port, because they can legitimately difffer. For example, when the receiver runs behind an nginx reverse proxy, port is the local bind port while pj_endpoint's port is the public one that ends up in the BIP21 URL.
4 participants
-> % payjoin-cli --bip78 --port 3000 --pj-endpoint https://example.com/ receive 50000 Error: If --port is 0, --pj-endpoint may not have a portTwo problems here:
--portis 3000, not 0.Pull Request Checklist
Please confirm the following before requesting review: